fix(popups): drop disabled criteria from segment criteria array#209
Open
dkoo wants to merge 3 commits into
Open
fix(popups): drop disabled criteria from segment criteria array#209dkoo wants to merge 3 commits into
dkoo wants to merge 3 commits into
Conversation
The segment editor saves number-based criteria with `{ min: 0, max: 0 }`
when the user toggles them off, and prior payloads can have a bare `0`
value. Those entries were persisted verbatim and localised straight into
`newspack_popups_view`, so the front-end matcher kept evaluating them as
active constraints (NPPM-2890).
Filter empty-valued criteria in `Newspack_Segments_Model` on both save
and read so existing affected segments are cleaned automatically and
future saves stay clean.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue in Newspack Popups segmentation where disabled criteria (notably number-range criteria saved as { min: 0, max: 0 } or other “empty” values) could still be persisted and then evaluated on the front end, incorrectly affecting segment membership and prompt eligibility.
Changes:
- Add defensive filtering in
Newspack_Segments_Modelto remove criteria whose values represent “no constraint” (on both read and save). - Apply the filter when reading segments (cleanup of legacy stored data) and when updating segments (prevent new bad data from being stored).
- Add PHPUnit regression and unit tests covering save-time filtering, read-time legacy cleanup, partial-range preservation, and malformed entry handling.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| plugins/newspack-popups/includes/class-newspack-segments-model.php | Filters out “empty/disabled” criteria on segment read and update via filter_criteria() / is_criteria_value_empty(). |
| plugins/newspack-popups/tests/test-segments.php | Adds regression/unit tests to ensure disabled/legacy criteria are dropped while valid/partial range criteria are preserved. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Address review feedback on PR #209: - Add `newspack_popups_is_criteria_value_empty` short-circuit and `newspack_popups_filter_segment_criteria` final filter so third-party criteria registered via `newspack_popups_default_criteria` can opt out of the default emptiness check. - Treat boolean `false` as an empty value (mirrors `is_disabled` convention) so future boolean criteria don't accidentally survive. - Split the int/float zero branches for clarity. - Tests: cover boolean handling, both extensibility filters, and verify the save path writes the filtered form to term meta (not just the read path). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
All Submissions:
Changes proposed in this Pull Request:
When a number-based segment criterion (e.g. "Articles read in session") is toggled off in the Newspack Campaigns segment editor, the editor still sent a
{ min: 0, max: 0 }value to the server. Older payloads can also have a bare0. Those entries were persisted verbatim and localised straight intonewspack_popups_viewon the front end, so the matching logic continued to evaluate them as active constraints — distorting segment membership and (per the originating report) preventing prompts from being shown to readers who should have qualified.This change adds a defensive filter in
Newspack_Segments_Modelthat drops criteria whose value carries no constraint —null, empty string, numeric0, empty array, or a (recursively) all-empty array/object. The filter is applied on both save (so new edits store clean criteria) and read (so existing affected segments are cleaned up automatically without publishers having to re-save every segment). Partially-disabled range criteria (e.g.{ min: 3, max: 0 }) are preserved.Closes NPPM-2890.
How to test the changes in this Pull Request:
3(or any non-zero value). Save the segment.newspack_popups_view.segmentsand find the entry for your segment — confirm the criteria array has the expected{ min: 3, max: 0 }(max remains disabled).newspack_popups_view.segmentsagain. Thearticles_read_in_sessioncriterion should no longer appear in the segment's criteria array.newspack_popups_view.segmentsonly shows thenewslettercriterion. Also open the segment in the editor and confirm the "Articles read in session" toggle is off (no stale0).Other information: